fix: update masternode rate limit on failed governance trigger path - #7521
fix: update masternode rate limit on failed governance trigger path#7521PastaPastaPasta wants to merge 3 commits into
Conversation
WalkthroughTrigger rate usage is recorded before governance trigger validation. Failed Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TriggerSubmitter
participant CGovernanceManager
participant RateLimiter
participant RelayScheduler
TriggerSubmitter->>CGovernanceManager: Submit trigger
CGovernanceManager->>RateLimiter: Record trigger usage
CGovernanceManager->>CGovernanceManager: Validate trigger
alt Valid trigger
CGovernanceManager->>RelayScheduler: Schedule future relay if required
else Failed trigger
CGovernanceManager->>CGovernanceManager: Mark for deletion
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🕓 Ready for review — 6 ahead in queue (commit 5b4cc0f) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The final code correctly advances the masternode rate buffer for failed triggers while deferring relay scheduling until after the trigger is retained, and the tests cover both behaviors. No correctness blockers remain, but two same-stack fixup commits should be folded into their originating commits to keep the history atomic and avoid preserving a known regression.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (completed)
🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:217a4097042>`:
- [SUGGESTION] <commit:217a4097042>:1: Squash the relay self-correction into the original fix
Commit 8862d235bb6 moves MasternodeRateUpdate() above the AddTrigger check while that function still inserts near-future triggers into setAdditionalRelayObjects. This creates a new intermediate regression where a malformed trigger marked for deletion can later be re-announced and served on GETDATA. Commit 217a4097042 correctly separates rate accounting from relay scheduling and adds targeted coverage, but it explicitly repairs behavior introduced by the preceding commit in this PR. Fold its governance and regression-test changes into 8862d235bb6 so the rate-limit fix is atomic and no retained commit contains the known relay-amplification regression.
In `<commit:39ba2d8ba97>`:
- [SUGGESTION] <commit:39ba2d8ba97>:1: Fold the audit-ID cleanup into the originating commits
Commit 39ba2d8ba97 only removes private audit identifiers from two comments introduced earlier in this same stack, making it a fixup commit under CONTRIBUTING.md's definition of commits that repeatedly change the same lines. It also leaves U009/U003 in the permanent messages of 8862d235bb6 and 217a4097042, so the stated cleanup remains incomplete. Amend the originating comments and commit messages to omit the identifiers, then drop this standalone cleanup commit.
39ba2d8 to
7c7bda9
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The final tree correctly advances the masternode rate buffer for failed triggers while scheduling deferred relay only after a trigger is retained, and the tests cover both paths. Carried forward from the prior review, two commit-history suggestions remain unresolved: the same-PR relay self-correction should be folded into the original fix, and the audit-ID cleanup should be folded into the commits that introduced those references. The latest delta is a pure rebase with identical PR patches, so no new defects or findings were introduced.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:6a36c89a4a6>`:
- [SUGGESTION] <commit:6a36c89a4a6>:1: Squash the relay self-correction into the original fix
Commit 643ba6d2068 moves MasternodeRateUpdate() above the AddTrigger check while deferred-relay scheduling is still a side effect of that function. At this intermediate commit, a malformed trigger rejected by AddTrigger and marked for deletion can still be queued in setAdditionalRelayObjects, later re-announced, and served on GETDATA. Commit 6a36c89a4a6 correctly extracts ScheduleAdditionalRelay() and calls it only after AddTrigger succeeds, but this repairs a regression introduced by the immediately preceding commit in the same PR. Fold the production correction and its relay regression coverage into the original fix so every retained commit contains the complete rate-limit fix without the known relay-amplification path.
In `<commit:7c7bda98220>`:
- [SUGGESTION] <commit:7c7bda98220>:1: Fold the audit-ID cleanup into the originating commits
Commit 7c7bda98220 only removes private U009/U003 identifiers from two comments introduced earlier in this same stack, making it an intra-stack fixup rather than an independent documentation change. The identifiers also remain in the permanent messages of commits 643ba6d2068 and 6a36c89a4a6. Amend the originating comments and commit messages to omit the private identifiers, then drop the standalone cleanup commit so the cleanup is complete and the history remains atomic.
7c7bda9 to
1d6dda8
Compare
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
LGTM; but this one is a bit interesting enough I want another's review
|
PR #7536 adds shared |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Independently verified: PR #7521 is now a single commit (1d6dda8) sitting directly on develop, containing the rate-buffer-update reordering, the ScheduleAdditionalRelay extraction gated to run only after AddTrigger succeeds, and both regression tests. The diff between the previously reviewed head (7c7bda9) and the current head is empty, confirming this is a history-only rewrite with no source changes. Both prior commit-hygiene findings are fixed: the self-correction is folded atomically into the fix, and no audit-ID identifiers remain in code, tests, or the commit message.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
AddGovernanceObjectInternal emplaced a trigger into mapObjects, attempted AddTrigger, and on failure returned early before MasternodeRateUpdate. MasternodeRateCheck treats a missing rate-buffer entry as allow, so a masternode that never lands a successful trigger was never rate-limited at all, defeating precisely the limiter meant to stop it. Each rejected trigger still cost a BLS verification and left a mapErasedGovernanceObjects entry retained for roughly 60 days on mainnet. Advance the rate buffer on the failure path too. Relay scheduling is extracted so a trigger just marked deleted is not added to the additional-relay set; that part is a self-correction for a regression this change would otherwise introduce, not a pre-existing bug.
1d6dda8 to
4e76c10
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e76c10a5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| int64_t nTimestamp = govobj.GetCreationTime(); | ||
| it->second.triggerBuffer.AddTimestamp(nTimestamp); | ||
| it->second.triggerBuffer.AddTimestamp(govobj.GetCreationTime()); |
There was a problem hiding this comment.
On testnet/devnet, where the superblock cycle is one hour, the accepted creation-time window spans three cycles (now - 2 * cycle through now + 1h), while the rate check permits a full buffer whenever its timestamp span exceeds 5 * cycle / 2.2, or about 2.27 cycles. A valid operator can therefore alternate malformed signed triggers between the window endpoints; every five-entry buffer retains both endpoints, GetRate() stays below dMaxRate, and every failed AddTrigger continues entering mapObjects. Regtest is similarly affected, so the consecutive-timestamp test passes while the flood remains unbounded on these networks. Record a non-attacker-controlled receipt time for failed attempts, or otherwise constrain this buffer, and test endpoint-spaced timestamps.
AGENTS.md reference: AGENTS.md:L166-L166
Useful? React with 👍 / 👎.
|
This pull request has conflicts, please rebase. |
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
LGTM; but this one is a bit interesting enough I want another's review
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
implementation is fine and regressions test idea is fine.
Though, impplementation of regressions tests requires re-working
| // message would then fan out to every peer, which in turn re-announce it. | ||
| BOOST_AUTO_TEST_CASE(failed_trigger_is_not_scheduled_for_additional_relay) | ||
| { | ||
| SetMockTime(1'700'000'000s); |
There was a problem hiding this comment.
why? regresions tests usually use 0 or now
1700000000 looks strange because it won't be updated every year anyway
There was a problem hiding this comment.
idk? does it matter?
There was a problem hiding this comment.
Fair point — switched these to SetMockTime(0s). The absolute epoch value wasn't load-bearing; we only need a deterministic clock inside the accepted rate-check window.
| COutPoint mn_outpoint; | ||
| CBLSSecretKey operator_key; | ||
|
|
||
| FailedTriggerRateSetup() : |
There was a problem hiding this comment.
there's not need to create new file and new FailedTriggerRateSetup testing environment.
Re-use src/test/governance_inv_tests.cpp, this file is almost identical copy of that one.
There was a problem hiding this comment.
Done: the regressions now live in src/test/governance_inv_tests.cpp and the standalone file is gone.
I kept a dedicated FailedTriggerRateSetup (TestChainSetup + ProRegTx + sync FINISHED) rather than reusing GovernanceInvSetup, because the INV fixture is MAIN/TestingSetup without a tip MN list or operator key. ProcessObject requires IsValidLocally (signature + DMN membership) before the failed-AddTrigger / rate-buffer path is exercised, so the INV fixture can't cover this regression.
There was a problem hiding this comment.
Follow-up: folded further so there is only one suite/fixture in this file — governance_inv_tests / GovernanceInvSetup. The complex DIP3 setup now also covers the INV/vote cases.
|
|
||
| /** Queue a deferred re-announcement for a trigger that is too new to propagate | ||
| * reliably yet. Only call this for objects we are keeping. */ | ||
| void ScheduleAdditionalRelay(const CGovernanceObject& govobj) |
There was a problem hiding this comment.
nit: it works with triggers only ; consider renaming it to ScheduleTriggerRelay or something similar for clarity. Edit comment also, it is not 'for objects' it is 'for triggers' only.
There was a problem hiding this comment.
Agreed — renamed to ScheduleTriggerRelay and updated the comments to say triggers only.
Move the failed-trigger rate regressions into governance_inv_tests.cpp, use SetMockTime(0s), and rename ScheduleAdditionalRelay to ScheduleTriggerRelay since it only applies to triggers.
|
Addressed @knst's review in a03a5e6:
Kept a separate Verified:
|
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/test/governance_inv_tests.cpp (1)
628-631: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the near-future offset from the named constants.
3550encodes the current values ofMAX_TIME_FUTURE_DEVIATIONandRELIABLE_PROPAGATION_TIME. If either constant changes,near_futuremay fall out of theScheduleTriggerRelay()arming window, and this test stops covering the deferred-relay regression. Compute the offset from the constants, and make thenow + 120readiness step depend onRELIABLE_PROPAGATION_TIMEas well.♻️ Proposed change to derive the offsets
- const int64_t near_future = now + 3550; + // Halfway inside the arming window so the test stays valid if either constant changes. + const int64_t near_future = now + count_seconds(MAX_TIME_FUTURE_DEVIATION) - + count_seconds(RELIABLE_PROPAGATION_TIME) / 2;- SetMockTime(std::chrono::seconds{now + 120}); + SetMockTime(std::chrono::seconds{now} + RELIABLE_PROPAGATION_TIME + 1s);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/governance_inv_tests.cpp` around lines 628 - 631, Update the near_future setup in the governance inventory test to derive its offset from MAX_TIME_FUTURE_DEVIATION and RELIABLE_PROPAGATION_TIME instead of the hard-coded 3550, keeping it inside the ScheduleTriggerRelay() arming window. Also replace the fixed now + 120 readiness step with an offset derived from RELIABLE_PROPAGATION_TIME so both timing assumptions track the named constants.src/governance/governance.cpp (1)
724-725: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse
GetAdjustedTime()for the addable-relay threshold.
ScheduleTriggerRelayarming usesGetTime(), butCheckPostponedObjectschecks validity and readiness forsetAdditionalRelayObjectswithGetAdjustedTime(). When the network offset is non-zero, the extra-relay decision can disagree with the drained readiness decision; useGetAdjustedTime()here to keep the same clock path.♻️ Proposed change
- if (govobj.GetCreationTime() > - GetTime() + count_seconds(MAX_TIME_FUTURE_DEVIATION) - count_seconds(RELIABLE_PROPAGATION_TIME)) { + const auto now{std::chrono::time_point_cast<std::chrono::seconds>(GetAdjustedTime())}; + if (govobj.CreationTime() > now + MAX_TIME_FUTURE_DEVIATION - RELIABLE_PROPAGATION_TIME) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/governance/governance.cpp` around lines 724 - 725, The future deviation check in CheckPostponedObjects uses GetTime() for the addable-relay threshold, but elsewhere in the same method GetAdjustedTime() is used for validity and readiness checks. When network offset is non-zero, this clock inconsistency causes the relay decision to disagree with the readiness decision. Replace GetTime() with GetAdjustedTime() in the govobj.GetCreationTime() comparison to ensure both relay-relay and readiness logic follow the same clock path.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/governance/governance.cpp`:
- Around line 724-725: The future deviation check in CheckPostponedObjects uses
GetTime() for the addable-relay threshold, but elsewhere in the same method
GetAdjustedTime() is used for validity and readiness checks. When network offset
is non-zero, this clock inconsistency causes the relay decision to disagree with
the readiness decision. Replace GetTime() with GetAdjustedTime() in the
govobj.GetCreationTime() comparison to ensure both relay-relay and readiness
logic follow the same clock path.
In `@src/test/governance_inv_tests.cpp`:
- Around line 628-631: Update the near_future setup in the governance inventory
test to derive its offset from MAX_TIME_FUTURE_DEVIATION and
RELIABLE_PROPAGATION_TIME instead of the hard-coded 3550, keeping it inside the
ScheduleTriggerRelay() arming window. Also replace the fixed now + 120 readiness
step with an offset derived from RELIABLE_PROPAGATION_TIME so both timing
assumptions track the named constants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 130bcbdd-d48b-46cf-ad6b-892c23b4710f
📒 Files selected for processing (3)
src/governance/governance.cppsrc/governance/governance.hsrc/test/governance_inv_tests.cpp
Fold the failed-trigger rate regressions into the single governance_inv_tests suite. GovernanceInvSetup now owns the DIP3 / ProRegTx path used by those cases and also covers the INV/vote tests.
|
Follow-up for suite naming / single-setup feedback in 5b4cc0f:
Verified: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/governance_inv_tests.cpp (1)
116-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the fixture teardown after leaving mock time set.
SetMockTime(0s)clears the mock clock globally; this comment does not match that effect. The tests set per-case mock time instead. Reset to0sfrom the destructor or remove the misleading fixture-level comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/governance_inv_tests.cpp` around lines 116 - 117, Update the test fixture around SetMockTime(0s) so teardown resets the global mock clock from its destructor, or remove the misleading fixture-level comment if teardown already performs the reset; keep per-case mock-time setup unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/test/governance_inv_tests.cpp`:
- Around line 116-117: Update the test fixture around SetMockTime(0s) so
teardown resets the global mock clock from its destructor, or remove the
misleading fixture-level comment if teardown already performs the reset; keep
per-case mock-time setup unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a1750e5a-7996-4083-a83c-7361dcf84048
📒 Files selected for processing (1)
src/test/governance_inv_tests.cpp
3 participants
Issue being fixed or feature implemented
In
AddGovernanceObjectInternal(), a trigger object is emplaced intomapObjects, thenAddTrigger()is attempted; on failure the function callsPrepareDeletion()and returns early - beforeMasternodeRateUpdate().MasternodeRateCheck()short-circuits with "allow" when the outpoint has no entry inmapLastMasternodeObject. So a masternode that never lands a successful trigger never gets a rate-buffer entry at all, and every subsequent malformed-but-signed trigger passes the rate check unbounded. The bypass defeats precisely the limiter designed to stop it.Each rejected trigger still costs a BLS verification, a
mapObjectsentry held for the deletion delay, agovernance.datwrite, and - after erasure - amapErasedGovernanceObjectsentry retained until roughly 60 days on mainnet. That last accumulator is the component that actually persists.Triggering requires a valid operator key for a masternode in the tip DMN list, and the victim must have requested the hash via INV. Objects are not relayed on this path, so there is no fan-out and the attacker must connect to each victim directly.
What was done?
MasternodeRateUpdate()above theAddTriggercheck so the rate buffer is advanced on the failure path too. The five-slot rate buffer then sticks, since it only advances on accept.The rate buffer cannot be gamed by spreading creation timestamps: the accepted timestamp window is narrower than the spread that would be needed to keep the computed rate below the maximum.
How Has This Been Tested?
Adds failed-trigger rate regressions in
src/test/governance_inv_tests.cppshowing the failed-trigger path must advance the masternode rate limit and must not schedule deferred trigger relay. Chain/ProRegTx plumbing uses the sharedsrc/test/util/masternode.hmodule from #7536.make -C src -j8 test/test_dash./src/test/test_dash --run_test=governance_failed_trigger_rate_tests,governance_inv_tests— passes.Remaining validation is delegated to CI on this PR.
Breaking Changes
None.
Checklist: